RFC: rework output configuration options and their consumption - #4966

I'm hoping to take a closer look at this over the weekend, but here are some immediate thoughts. I'm not very familiar with the details of the current config-render-scene pipeline, so perhaps these are all obvious or irrelevant!

• I really like the very explicit config model. IMO, Manim should do as little magic behind the scenes as possible, so this works well.
• If I'm understanding you correctly, a render session is initially built from the provided config options. This session object is passed to the scene that is to be rendered, which then builds a session spec and uses that to provide output somehow. How does this work for rendering multiple scenes?
• Something feels a bit off wrt the relationship between a scene and its config. Some things are obviously necessary for the scene to know what do to (camera size, frame rate, LaTeX templates, etc.). But does a scene really need to know things like output format or whether the file browser will be opened at the end? Those feel like things that should be owned by the renderer or a manager.

Exciting stuff so far 👍

Great questions!

• As far as the design reaches right now, when rendering multiple scenes each scene would get a separate manager (and as an extension also a separate render session spec). When using the CLI (manim render ...), each call constructs a new manager with an independent spec etc.
• You are absolutely right: the scene should not own output settings etc. at all! This is just a transitional design and makes sense because we more or less had it like that before -- but in a few PRs, handling these settings will indeed be taken away from the scene and moved to the manager.

Great questions!

• As far as the design reaches right now, when rendering multiple scenes each scene would get a separate manager (and as an extension also a separate render > session spec). When using the CLI (manim render ...), each call constructs a new manager with an independent spec etc.
• You are absolutely right: the scene should not own output settings etc. at all! This is just a transitional design and makes sense because we more or less had > it like that before -- but in a few PRs, handling these settings will indeed be taken away from the scene and moved to the manager.

Cool, I thought that might be the case! In my mind, a "session" would be the execution and rendering of all scenes provided by the user, but that's just nomenclature stuff. I'm also thinking about this from a non-CLI perspective (let's say I'm building an editor for Manim which builds its own manager or whatever), but none of this seems to directly preclude doing that, so I'm happy there.

Another off-the-cuff thought before I start looking at the code: Maybe I'm an extreme outlier and my workflow shouldn't weigh too heavily in these considerations, but I actually find it very useful to be able to invoke with tempconfig inside a scene. Here's an example of a dumb thing I'm currently doing to reuse scenes inside another scene:

class CombinedScene(Scene):
    def construct(self):
        square = Square()
        self.play(FadeIn(square))
        self.play(FadeOut(square))
        # this scene is very dense, so turn off caching
        with tempconfig(dict(disable_caching=True)):
            ReusableScene.construct(self)
        self.play(FadeOut(*self.mobjects))

Obviously I'll survive if I have to do something else! There's also this workaround which again isn't strictly necessary, but definitely a nice thing to have at your disposal.
Maybe there's a way to construct a temporary config on top of the current frozen one, so we don't mutate state but rather construct a data structure of stacked configs - sooort of similar to how setdefault works for mobjects and animations.

OK, as far as I can tell everything looks good on the code side. I have a few comments about config options:

• Both the implementation and docs emphasize that if a video format is requested but the scene contains no play calls, then the renderer should produce a "useful still image", i.e., render a last-state PNG. Is this really the behavior we'd want?

  Part of the philosophy behind this PR is that a) configs are immutable once the scene begins and b) Manim should very rarely try and "fix" the user's config options, and this behavior is effectively the same as changing the config's output format to PNG. I think that's 100% fine if format = auto, but if the user has explicitly specified e.g. format = mp4, then that feels a little iffy. Here are two alternate options for when format = [video] but there are no play calls:

  1. Assume the user has done this on purpose and output the last (and only) frame of the scene to an mp4 (or whatever format the user requested). I don't know if this is technically possible or why someone would ever wanna do this, but it feels "honest" in the same way the new config design does.
  2. Keep the current behavior and output a PNG instead of a video, but also log a message or warning à la No animation frames were produced in {scene_name}. The output has been saved as an image instead.. Then the user is explicitly informed that we're going against the wishes stated in the config.

• Currently, preview and show_in_file_browser are mutually exclusive. Is there any reason for this? I could easily see a user wanting both Finder and VLC to open when the render finishes. Order of opening could be determined either by the order the flags are provided, or we could pick one winner (always having the video player/image viewer open above the file system feels right to me).

• Similarly, I'm not completely convinced that save_last_frame should set format = png. It seems perfectly reasonable for a user to want to render a video and a final-state PNG for, say, thumbnails. I could see it being an execution intent similar to dry-run: the scene behaves as it normally would based on the output format, but the final output is augmented by the execution intent. They also both have a similar canceling-out case: setting dry-run=True, output=none does the "same thing" twice, and save_last_frame=True, output=png does the same thing twice. If this isn't technically feasible, I think the current behavior is fine as well :)

As a side note, here's a possibly overengineered thing I'm wondering: would it perhaps make sense to encode the constraints of the config (e.g. not (transparent is True and format == 'mp4') and not (preview is True and format == 'none')) and store it in a SessionConstraints class or similar? This would provide a single source of truth for validation, and it might make for easy testing since we could exhaustively test that no invalid combinations of options are allowed. This is obviously easy to add later, so no worries if it's out of scope for this PR.

OK, as far as I can tell everything looks good on the code side. I have a few comments about config options:

Thanks for the careful review, much appreciated!

• Both the implementation and docs emphasize that if a video format is requested but the scene contains no play calls, then the renderer should produce a "useful still image", i.e., render a last-state PNG. Is this really the behavior we'd want?
  Part of the philosophy behind this PR is that a) configs are immutable once the scene begins and b) Manim should very rarely try and "fix" the user's config options, and this behavior is effectively the same as changing the config's output format to PNG. I think that's 100% fine if format = auto, but if the user has explicitly specified e.g. format = mp4, then that feels a little iffy. Here are two alternate options for when format = [video] but there are no play calls:

  1. Assume the user has done this on purpose and output the last (and only) frame of the scene to an mp4 (or whatever format the user requested). I don't know if this is technically possible or why someone would ever wanna do this, but it feels "honest" in the same way the new config design does.
  2. Keep the current behavior and output a PNG instead of a video, but also log a message or warning à la No animation frames were produced in {scene_name}. The output has been saved as an image instead.. Then the user is explicitly informed that we're going against the wishes stated in the config.

Ah, this is a great question! I tend to agree that any "smartness" in the handling of the output format should be restricted to when the output format is set to "auto". The current implementation more or less simply mimics the behavior on the main branch, but I wouldn't mind introducing another breaking change here.

There actually is an argument that could be made for the library always appending a 1-frame long animation at the end (which would resolve the issue reported many times that for a scene just containing a self.play(some_animation, run_time=1) call and nothing afterwards the last frame is not showing correctly, but it would mean that the video length would no longer be the sum of the run times in the scene, but the run time sum plus one frame instead -- but I feel like this is something that we should discuss a bit more broadly, and I don't want to hide something like this in the otherwise already massive PR.

I think what I'd like to do is implement a combination of your two suggestions: only let manim try to be smart about the output format while the value is "auto" (the default), raise an error when a video is requested in a scene without animations, and log a warning when "auto" resolves to an image instead of a video. Thoughts?

• Currently, preview and show_in_file_browser are mutually exclusive. Is there any reason for this? I could easily see a user wanting both Finder and VLC to open when the render finishes. Order of opening could be determined either by the order the flags are provided, or we could pick one winner (always having the video player/image viewer open above the file system feels right to me).

Are you sure? This might just be explained incorrectly in the docs, the code just has two sequential if branches, not an if/elif.

• Similarly, I'm not completely convinced that save_last_frame should set format = png. It seems perfectly reasonable for a user to want to render a video and a final-state PNG for, say, thumbnails. I could see it being an execution intent similar to dry-run: the scene behaves as it normally would based on the output format, but the final output is augmented by the execution intent. They also both have a similar canceling-out case: setting dry-run=True, output=none does the "same thing" twice, and save_last_frame=True, output=png does the same thing twice. If this isn't technically feasible, I think the current behavior is fine as well :)

It's an interesting suggestion, and sort of prompts the question whether it should be allowed to specify multiple output formats at once. I tend to agree, but would rather want to implement this as a fancy new feature later, separately from this refactor.

As a side note, here's a possibly overengineered thing I'm wondering: would it perhaps make sense to encode the constraints of the config (e.g. not (transparent is True and format == 'mp4') and not (preview is True and format == 'none')) and store it in a SessionConstraints class or similar? This would provide a single source of truth for validation, and it might make for easy testing since we could exhaustively test that no invalid combinations of options are allowed. This is obviously easy to add later, so no worries if it's out of scope for this PR.

Interesting. I am sort of satisfied with the restrictions being all spelled out in OutputSpec.__post_init__ -- but perhaps something for a later extension of the system when we, say, let users also pass custom encoder config options?

I'll push a commit to change the behavior overriding user intent with the output format when there are no animations, plus fix the wording regarding preview + open in file browser. Thanks again!

I think what I'd like to do is implement a combination of your two suggestions: only let manim try to be smart about the output format while the value is "auto" (the default), raise an error when a video is requested in a scene without animations, and log a warning when "auto" resolves to an image instead of a video. Thoughts?

Yep, that's a very nice intermediary step!

Are you sure? This might just be explained incorrectly in the docs, the code just has two sequential if branches, not an if/elif.

Oop you're right! Both work at the same time on my machine. I had interpreted open_file as setting a variable for later, so I thought the latter in_browser argument would overwrite the former. Then it's just docs that need to be fixed, thanks!

sort of prompts the question whether it should be allowed to specify multiple output formats at once. I tend to agree, but would rather want to implement this as a fancy new feature later, separately from this refactor.

Agreed! I do like the idea of save_last_frame as execution intent along with live-preview and dry-run though. I think that class of behavior is interesting! Might be possible to expose hooks for user-written execution intents down the line.

Interesting. I am sort of satisfied with the restrictions being all spelled out in OutputSpec.post_init -- but perhaps something for a later extension of the system when we, say, let users also pass custom encoder config options?

I was envisioning __post_init__ using the constraint class to perform its validation, but again this is easily doable later.

Pushed changes as discussed and fixed one test that failed as a consequence. I have also added a couple more cheap tests to make sure the behaviour is exactly what we want for now. (Need a lot of tests for the upcoming file writer decoupling and renderer migrations.)